[DFC Orders] Sync remote products when order cycle opens #13167

dacook · 2025-02-19T03:49:28Z

ℹ️ Funded Feature. Please track ALL ASSOCIATED WORK under the associated tracking code `#11678 DFC Orders`

What? Why?

Closes [DFC Products] Ensure Product details are synced at correct intervals #12986

When an order cycle opens, any remote (DFC) products will get re-imported to ensure the product is up to date. This is because the product on OFN is considered a cache, or a view of, the "real" product.
There's currently no record or notification of this happening for the enterprise managers or OC co-ordinator.

This required some changes to the way we enqueue and execute the scheduled jobs for opening an order cycle.

There is also a slight change to the Webhook order_cycle.opened (https://ofn-user-guide.gitbook.io/ofn-api-handbook/webhooks/event-types). The at timestamp now more accurately reflects the time the event occurred, rather than when the webhook was sent. It's such a nuanced change I don't think it's necessary to announce.

What should we test?

Import a product from DFC (see example at [DFC Orders] List products to import on screen #13125 (comment))
- Note: the product must be imported to an enterprise that you are the owner of.
Edit some of the details, so that it becomes out of sync with the original source. (eg variant name, price, etc)
Add the product to an order cycle and schedule it to open soon
After the order cycle is opened, the details should be updated.

Release notes

Changelog Category (reviewers may add a label for the release notes):

User facing changes
API changes (V0, V1, DFC or Webhook)
Technical changes only
Feature toggled

The title of the pull request will be included in the release notes.

mkllnk

Yes, all going in the right direction. That's basically it. You just need to check for other open order cycles of the product, maybe. Or maybe we just skip that until it becomes a problem? I'm worried that multiple enterprises could have different overlapping OCs and then this will never get triggered. 🤷 But maybe that's a problem we don't have yet. 😉

app/jobs/order_cycle_opened_job.rb

spec/jobs/order_cycle_opened_job_spec.rb

dacook · 2025-02-24T00:21:03Z

app/jobs/order_cycle_opened_job.rb

-  def mark_as_opened(order_cycles)
-    now = Time.zone.now
-    order_cycles.update_all(opened_at: now, updated_at: now)
-  end


There's a spec for this in a later commit.

I considered pre-loading the variant and product with includes, But can't do that here because it's a polymorphic relationship.

It was probably better to be explicit at each test, but this one is always repeated and approaches the line length, so I wanted to just define it once at the top.

There might be a delay before it gets sent, so it's better to record the time the event occurred at. It would have been simpler to just add it to the data hash, but I felt it was an important detail for an event and should be at the top level along with event name. In the case of order cycle opening, this is the same as opened_at. I've included this in the payload for clarity too.

And re-organise specs. TOFIX: concurrency test now fails. why?

Being based on the DB value should be more robust. This prevents an order cycle from being "opened" when it's already open. But note that the order cycle can become "unopened" (see OrderCycle#reset_opened_at). Nice to see the database query count drop, but I must confess I don't know why!

and in that case, might as well define it once at the top. But it didn't help with spec failures.. see next commit

I guess we lose some of the detail in the db.

mkllnk

Nice work. I lost some focus towards the end because it's after 5pm. But it's mostly good. I think there are a few small things to improve and then it's good.

app/jobs/open_order_cycle_job.rb

app/jobs/order_cycle_opened_job.rb

mkllnk · 2025-02-25T05:54:02Z

spec/jobs/open_order_cycle_job_spec.rb

+    before do
+      user.oidc_account.update!(token: allow_token_for(email: user.email))
+    end
+
+    # should we move any parts of importing to a separate class, and test it separately?
+    it "synchronises products from a FDC catalog", vcr: true do
+      user.update!(oidc_account: build(:testdfc_account))
+      # One product is existing in OFN


I think that you want to create(:testdfc_user). The allow_token_for method from the AuthorizationHelper is to create a valid access token locally for the OFN DFC API. If you use VCR to record a real interaction then you will have real tokens and don't need that helper.

Oops, thanks.
I have to admit I cheated and copy/pasted the VCR fixture from elsewhere. I can't re-record it because I'm forbidden. Can you please guide me how to authenticate it?

I wonder how to best document this. You need a refresh token. So the best way I found is to run your local dev server, connect a user to the testdfc user OIDC account and then get the token:

./bin/rails runner 'puts "OPENID_REFRESH_TOKEN=\"#{OidcAccount.last.refresh_token}\""'

You then paste that line into your .env.test.local file. You also need the OIDC creds in your env:

OPENID_APP_ID="coopcircuits" OPENID_APP_SECRET="..." OPENID_REFRESH_TOKEN="eyJh..."

I think that we should put these instructions as a comment in .env.test.

Thank you! You've told me this before and I forgot sorry. I'll push up a suggested .env.test change.

app/jobs/order_cycle_opened_job.rb

mkllnk · 2025-02-25T06:02:30Z

app/jobs/open_order_cycle_job.rb

+class OpenOrderCycleJob < ApplicationJob
+  def perform(order_cycle_id)
+    order_cycle = OrderCycle.find(order_cycle_id)
+    sync_remote_variants(order_cycle)


I imagined the syncing in a new job. Then that logic is isolated and can be triggered via buttons or at other points, too. But I think that you wanted to make sure it finishes before the order cycle is seen as open. Fair enough. There is perform_now though which can be handy.

I agree that it might be useful in the future but I didn't think it was needed yet.

Open Order Cycle still needs its own job here, so that the syncing (which has to happen synchronously) doesn't delay the opening of other order cycles.

The naming is confusingly similar to the existing OrderCycleOpenedJob, but let's see if we can improve that one..

mkllnk · 2025-02-25T06:06:23Z

app/jobs/order_cycle_opened_job.rb

-# Currently, an order cycle is considered open in the shopfront when orders_open_at >= now.
-# But now there are some pre-conditions for opening an order cycle, so we would like to change that.
-# When it changes, this would become OrderCycleOpeningJob, with the responsibility of opening each
-# order cycle by setting opened_at = now.
 class OrderCycleOpenedJob < ApplicationJob


This name is now confusing because it happens after orders_open_at but before opened_at. So the OC hasn't really opened yet. Most of my ideas are still confusing. Maybe FindOrderCyclesToOpenJob? 🤷

Yeah I'm not sure either.. how about:

EnqueueOpeningOrderCyclesJob ScheduleOpeningOrderCyclesJob ProcessOpeningOrderCyclesJob OpeningOrderCyclesJob ScheduleOrderCyclesToOpenJob

I think I like the last one, what do you think?

I like the first and the last. I would go for Enqueue because it doesn't schedule for a future time. It just does it straight after. It could even happen in parallel because Sidekiq uses several threads. It's really hard to write though. Maybe trigger is a better word? In that case we could also say TriggerOrderCycleOpeningJob. But then it's better the re-use the other job name: TriggerOpenOrderCycleJob. I also like the ToOpen because it refers to the action to do. My favourites now are:

TriggerOpeningOrderCyclesJob TriggerOpenOrderCycleJob TriggerOrderCyclesToOpenJob

The last one is my favourite. But I would also be happy about ScheduleOrderCyclesToOpenJob if you prefer the schedule part.

Thanks! Trigger sounds good, I'll go ahead with TriggerOrderCyclesToOpenJob

app/jobs/open_order_cycle_job.rb

mkllnk · 2025-02-25T06:13:35Z

spec/jobs/open_order_cycle_job_spec.rb

+      .to change { order_cycle.opened_at }
+
+    expect(order_cycle.opened_at).to be_within(1).of(now)


Hm, I thought that there was some way to convert the timestamp to the db precision. I can't remember it now.

Yeah this was just the first solution I came across, and it worked :)
Another thought was to use to_i when comparing.

Co-authored-by: Maikel <[email protected]>

> The order cycle itself is not changed. It's just that time passed and it's now considered open/closed.

dacook · 2025-02-26T00:08:41Z

@mkllnk thanks for your feedback. I've made some changes and have a couple of questions.

mkllnk · 2025-02-26T02:56:55Z

app/jobs/open_order_cycle_job.rb

+    rescue ActiveRecord::RecordNotFound
+      # do nothing


This shouldn't really happen, should it? I'm wondering if we should report this to Bugsnag. Otherwise there may be a case when we are wondering: Why did the job not do its job? And we won't have any record of that.

I think that this only happens if the syncing takes so long that five minutes passed and the job gets scheduled again.

You're right, we shouldn't suppress the error.

Because Sidekiq retries jobs that raise an exception (for about 20 days), I chose to suppress the error.
I also don't want to set sidekiq_options retries: 0, in case there's some other error, and maybe it should be retried (although maybe only for 10 mins). Sidekiq doesn't seem to have a way to configure retries based on a condition.

So.. I'm thinking to set sidekiq_options retry_for: 10.minutes and not have any error handling. What do you think?

And document how to run it.

After 10 minutes, I'd consider that it failed to open the order cycle. Who would want their products to sync, or get a notification at a random time during the order cycle? Best viewed with whitespace ignored.

This name better reflects what it's doing. As this job is scheduled automatically by Sidekiq, I think there shouldn't be any jobs with the old name in redis. So I didn't bother keeping a placholder for the old name.

github-actions bot assigned dacook Feb 19, 2025

dacook force-pushed the sync-products-on-oc-opened-12986 branch 2 times, most recently from 4d8cd8c to f665516 Compare February 19, 2025 04:22

dacook requested a review from mkllnk February 19, 2025 04:22

This comment was marked as resolved.

Sign in to view

mkllnk reviewed Feb 20, 2025

View reviewed changes

app/jobs/order_cycle_opened_job.rb Outdated Show resolved Hide resolved

app/jobs/order_cycle_opened_job.rb Outdated Show resolved Hide resolved

app/jobs/order_cycle_opened_job.rb Outdated Show resolved Hide resolved

spec/jobs/order_cycle_opened_job_spec.rb Outdated Show resolved Hide resolved

dacook force-pushed the sync-products-on-oc-opened-12986 branch 2 times, most recently from ef5ce63 to 2d4c5d1 Compare February 24, 2025 00:20

dacook commented Feb 24, 2025

View reviewed changes

dacook force-pushed the sync-products-on-oc-opened-12986 branch 2 times, most recently from 06088fd to 42ddb61 Compare February 24, 2025 01:28

dacook changed the title ~~Sync products when Order Cycle opens~~ Sync products when order cycle opens Feb 24, 2025

dacook changed the title ~~Sync products when order cycle opens~~ Sync remote products when order cycle opens Feb 24, 2025

dacook added 8 commits February 24, 2025 13:57

Synchronise remote products when order cycle opens

6aa2c14

I considered pre-loading the variant and product with includes, But can't do that here because it's a polymorphic relationship.

Mark each order cycle as opened once pre-conditions have been processed

a5b98ca

Refactor spec

9b9612d

It was probably better to be explicit at each test, but this one is always repeated and approaches the line length, so I wanted to just define it once at the top.

Move to new job

7ea5b30

And re-organise specs. TOFIX: concurrency test now fails. why?

Explicitly set timecop freeze time

9079fbd

and in that case, might as well define it once at the top. But it didn't help with spec failures.. see next commit

Reduce specificy of test

65d8c56

I guess we lose some of the detail in the db.

dacook force-pushed the sync-products-on-oc-opened-12986 branch from 166ed5c to 65d8c56 Compare February 24, 2025 03:03

dacook marked this pull request as ready for review February 24, 2025 03:03

dacook changed the title ~~Sync remote products when order cycle opens~~ [DFC Orders] Sync remote products when order cycle opens Feb 24, 2025

dacook requested a review from mkllnk February 25, 2025 03:08

mkllnk added the user facing changes Thes pull requests affect the user experience label Feb 25, 2025

mkllnk requested changes Feb 25, 2025

View reviewed changes

dacook and others added 2 commits February 26, 2025 10:00

Use existing method

8b11b2e

Co-authored-by: Maikel <[email protected]>

Only update one timestamp

f0fbb57

> The order cycle itself is not changed. It's just that time passed and it's now considered open/closed.

dacook force-pushed the sync-products-on-oc-opened-12986 branch from 2eb96bc to e2a871e Compare February 26, 2025 00:08

dacook requested a review from mkllnk February 26, 2025 00:08

mkllnk reviewed Feb 26, 2025

View reviewed changes

dacook added 2 commits February 26, 2025 15:34

Use correct testing methods

980a24b

Re-record fixture

2f4b81c

And document how to run it.

dacook force-pushed the sync-products-on-oc-opened-12986 branch from e2a871e to 0fa71e2 Compare February 26, 2025 04:39

dacook added 3 commits February 26, 2025 16:09

Raise error if record not found, and don't retry too many times

9220f71

After 10 minutes, I'd consider that it failed to open the order cycle. Who would want their products to sync, or get a notification at a random time during the order cycle? Best viewed with whitespace ignored.

Rename OrderCycleOpenedJob

e6a7a87

This name better reflects what it's doing. As this job is scheduled automatically by Sidekiq, I think there shouldn't be any jobs with the old name in redis. So I didn't bother keeping a placholder for the old name.

Clean up unused include

6d5aaa1

dacook force-pushed the sync-products-on-oc-opened-12986 branch from f2a0a6d to 6d5aaa1 Compare February 26, 2025 05:09

dacook requested a review from mkllnk February 26, 2025 05:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DFC Orders] Sync remote products when order cycle opens #13167

[DFC Orders] Sync remote products when order cycle opens #13167

dacook commented Feb 19, 2025 •

edited

Loading

This comment was marked as resolved.

mkllnk left a comment

dacook Feb 24, 2025

mkllnk left a comment

mkllnk Feb 25, 2025

dacook Feb 26, 2025

mkllnk Feb 26, 2025

dacook Feb 26, 2025

mkllnk Feb 25, 2025

dacook Feb 25, 2025

mkllnk Feb 25, 2025

dacook Feb 25, 2025

mkllnk Feb 26, 2025

dacook Feb 26, 2025

mkllnk Feb 25, 2025

dacook Feb 25, 2025

dacook commented Feb 26, 2025

mkllnk Feb 26, 2025

dacook Feb 26, 2025

		.to change { order_cycle.opened_at }

		expect(order_cycle.opened_at).to be_within(1).of(now)

[DFC Orders] Sync remote products when order cycle opens #13167

Are you sure you want to change the base?

[DFC Orders] Sync remote products when order cycle opens #13167

Conversation

dacook commented Feb 19, 2025 • edited Loading

ℹ️ Funded Feature. Please track ALL ASSOCIATED WORK under the associated tracking code #11678 DFC Orders

What? Why?

What should we test?

Release notes

This comment was marked as resolved.

mkllnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacook commented Feb 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacook commented Feb 19, 2025 •

edited

Loading

ℹ️ Funded Feature. Please track ALL ASSOCIATED WORK under the associated tracking code `#11678 DFC Orders`